[#1668] API types to simplify work with launch configuration attributes #1669

ruspl-afed · 2024-12-19T15:19:08Z

identify launch attribute
connect it with preference metadata (to supply defaults/label/description)
read attribute from configuration
write attribute to configuration working copy
demonstrate usage for ExternalTools

eclipse-platform-bot · 2024-12-19T15:23:34Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From eaa3ac17f344b48134ddc5cebf426b2d852af7f2 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Thu, 30 Jan 2025 10:48:00 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF b/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
index 169587e22e..2be9d48ae2 100644
--- a/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
+++ b/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-Localization: plugin
 Bundle-SymbolicName: org.eclipse.core.externaltools;singleton:=true
-Bundle-Version: 1.3.400.qualifier
+Bundle-Version: 1.3.500.qualifier
 Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.29.0,4.0.0)",
  org.eclipse.debug.core;bundle-version="[3.9.0,4.0.0)",
  org.eclipse.core.variables;bundle-version="[3.2.800,4.0.0)"
-- 
2.48.1

Further information are available in Common Build Issues - Missing version increments.

github-actions · 2024-12-19T15:46:35Z

Test Results

1 758 files ±0 1 758 suites ±0 1h 23m 54s ⏱️ - 1m 22s
4 174 tests ±0 4 151 ✅ ±0 23 💤 ±0 0 ❌ ±0
13 119 runs ±0 12 952 ✅ ±0 167 💤 ±0 0 ❌ ±0

Results for commit 8beaf15. ± Comparison against base commit 0fc54b8.

♻️ This comment has been updated with latest results.

mickaelistria · 2024-12-19T15:53:59Z

As mentioned in the call, some example using this API proposal would be helpful. There is probably some content in org.eclipse.ui.externaltools/External Tools Base/org/eclipse/ui/externaltools/internal/launchConfigurations you can tweak to leverage this API.

ruspl-afed · 2025-01-30T12:15:58Z

I'm not sure why it is is failing @HannesWell

11:59:41.178 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify (verify) on project org.eclipse.debug.core: 
Execute ApiApplication failed: InvocationTargetException: EmptyStackException -> [Help 1]

HannesWell · 2025-01-30T20:04:07Z

I'm not sure why it is is failing @HannesWell

11:59:41.178 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify (verify) on project org.eclipse.debug.core: 
Execute ApiApplication failed: InvocationTargetException: EmptyStackException -> [Help 1]

It looks like your change is breaking API-tools^^
I have replayed the Jenkins build the -e set to get an more informative stack-trace:
https://ci.eclipse.org/platform/job/eclipse.platform/job/PR-1669/7/

ruspl-afed · 2025-01-31T06:56:53Z

Thank you @HannesWell for your assistance.

It looks like your change is breaking API-tools^^

WOW! But that wasn't part of my plan. It looks like this issue was already reported. Interesting, may be I can have a deeper look to it soon.

And still interested in your feedback regarding PR content.

ruspl-afed · 2025-02-01T06:32:01Z

After supporting record for API Tools everything is green @HannesWell

ruspl-afed · 2025-02-04T06:43:49Z

ok, a few more days for discussion and I'm going to merge it

HannesWell · 2025-02-05T00:37:44Z

After supporting record for API Tools everything is green @HannesWell

Thank you for that fix/enhancement of API-tools! It's much appreciated.

And still interested in your feedback regarding PR content.

Sorry for not responding. It has been some busy days recently. I have not forgotten this, just didn't have the time for it.
I try my best to have look at it in the next days.

* identify launch attribute * connect it with preference metadata (to supply defaults/label/description) * read attribute from configuration * write attribute to configuration working copy * demonstrate usage for ExternalTools

HannesWell

I had a more detailed look at it and understand the goal and see the benefits from this: Better type-safety and more convenient read and writes.
Nevertheless I'm not sure that these benefits really carry the weight of the change, because currently the new API surface is very big.
Below I made some suggestion to make the API more compact, maybe we can then achieve a sufficiently got relation of advantages to API surface.

With these suggestions we would end up with just the LaunchAttributeIdentity and LaunchAttributeDefined types. Maybe we could even 'inline' the former into the latter and would then still get benefits mentioned above.
And eventually just having an ILaunchAttribute interface added with a few static factories to create them is something that could indeed be a in my opinion suitable addition.

HannesWell · 2025-02-07T23:37:58Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LauchAttributeIdentityRecord.java

+public record LauchAttributeIdentityRecord(String id) implements LaunchAttributeIdentity {
+
+	/**
+	 * Convenience way to compose full qualified name for launch attribute
+	 *
+	 * @param qualifier usually corresponds to Bundle-Symbolic-Name
+	 * @param key short key to name this very attribute in the scope of
+	 *            qualifier
+	 */
+	public LauchAttributeIdentityRecord(String qualifier, String key) {
+		this(qualifier + "." + key); //$NON-NLS-1$
+	}
+
+}


This could be a local record in a static factory method on the LaunchAttributeIdentity type? Then this impl would be fully internal and we don't have to repeat the type in the name.

And this type name has a typo: Lauch instead of Launch.

HannesWell · 2025-02-07T23:46:17Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeDefined.java

+ *
+ * @since 3.23
+ */
+public interface LaunchAttributeDefined<V> {


In my opinion this would better be named either

Suggested change

public interface LaunchAttributeDefined<V> {

public interface LaunchAttributeDefinition<V> {

or just

Suggested change

public interface LaunchAttributeDefined<V> {

public interface LaunchAttribute<V> {

HannesWell · 2025-02-07T23:49:38Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeProbe.java

+ *
+ * @since 3.23
+ */
+public final class LaunchAttributeProbe<V> implements Supplier<Optional<V>> {


Is this class really necessary? To me it looks like a wrapper of a LaunchAttributeRead that takes care of the exception handling.
I suggest to let LaunchAttributeRead.get/read() return that Optional<V>

HannesWell · 2025-02-07T23:53:12Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeRead.java

+	public V get() throws CoreException {
+		if (String.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, String.class.cast(value.get())));
+		}
+		if (Boolean.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, Boolean.class.cast(value.get())));
+		}
+		if (Integer.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, Integer.class.cast(value.get())));
+		}
+		if (List.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, List.class.cast(value.get())));
+		}
+		if (Map.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, Map.class.cast(value.get())));
+		}
+		if (Set.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, Set.class.cast(value.get())));
+		}
+		throw new CoreException(Status.error(id, new ClassCastException()));
+	}


Couldn't we avoid this entire class when instead this method would be added as a read(ILaunchConfiguration) method to LaunchAttributeDefined?

HannesWell · 2025-02-07T23:54:42Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeWrite.java

+	public void accept(ILaunchConfigurationWorkingCopy working) {
+		if (String.class.equals(type)) {
+			working.setAttribute(id, String.class.cast(value.get()));
+		} else if (Integer.class.equals(type)) {
+			working.setAttribute(id, Integer.class.cast(value.get()).intValue());
+		} else if (Boolean.class.equals(type)) {
+			working.setAttribute(id, Boolean.class.cast(value.get()).booleanValue());
+		} else if (List.class.equals(type)) {
+			working.setAttribute(id, List.class.cast(value.get()));
+		} else if (Map.class.equals(type)) {
+			working.setAttribute(id, Map.class.cast(value.get()));
+		} else if (Set.class.equals(type)) {
+			working.setAttribute(id, Set.class.cast(value.get()));
+		} else {
+			working.setAttribute(id, value.get());
+		}
+	}


Same here:
Couldn't we avoid this entire class when instead this method would be added as a write(ILaunchConfiguration) and write(ILaunchConfiguration, V value) method to LaunchAttributeDefined?

HannesWell · 2025-02-08T00:02:24Z

...c/org/eclipse/core/externaltools/internal/launchConfigurations/LaunchAttributeArguments.java

+import org.eclipse.debug.core.LaunchAttributeDefined;
+import org.eclipse.debug.core.LaunchAttributeIdentity;
+
+public final class LaunchAttributeArguments implements LaunchAttributeDefined<String> {


Couldn't we replace this entire class by a static field LaunchAttributeDefined<String>LAUNCH_ATTRIBUTE_ARGUMENTS e.g in the IExternalToolConstants class? Implemented either with an anonymous implementation for each field or a static factory method (that contains it).
I wonder the same for the companion LaunchAttributeDefined implementations.

HannesWell · 2025-02-08T00:09:57Z

debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeRead.java

+		if (String.class.equals(type)) {
+			return type.cast(configuration.getAttribute(id, String.class.cast(value.get())));
+		}


Couldn't we have this if-chain in the constructor and then encapsulate the action into a lambda and just execute it here?

ruspl-afed force-pushed the 1668 branch 3 times, most recently from ceac906 to 0eaee71 Compare January 30, 2025 11:38

ruspl-afed force-pushed the 1668 branch from 5740dd9 to 35d8960 Compare February 1, 2025 05:55

HannesWell force-pushed the 1668 branch from 35d8960 to 8beaf15 Compare February 7, 2025 23:30

HannesWell requested changes Feb 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1668] API types to simplify work with launch configuration attributes #1669

[#1668] API types to simplify work with launch configuration attributes #1669

ruspl-afed commented Dec 19, 2024 •

edited

Loading

eclipse-platform-bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

mickaelistria commented Dec 19, 2024

ruspl-afed commented Jan 30, 2025

HannesWell commented Jan 30, 2025

ruspl-afed commented Jan 31, 2025

ruspl-afed commented Feb 1, 2025

ruspl-afed commented Feb 4, 2025

HannesWell commented Feb 5, 2025

HannesWell left a comment

HannesWell Feb 7, 2025

HannesWell Feb 7, 2025

HannesWell Feb 7, 2025

HannesWell Feb 7, 2025

HannesWell Feb 7, 2025

HannesWell Feb 8, 2025

HannesWell Feb 8, 2025

	public interface LaunchAttributeDefined<V> {
	public interface LaunchAttributeDefinition<V> {

	public interface LaunchAttributeDefined<V> {
	public interface LaunchAttribute<V> {

[#1668] API types to simplify work with launch configuration attributes #1669

Are you sure you want to change the base?

[#1668] API types to simplify work with launch configuration attributes #1669

Conversation

ruspl-afed commented Dec 19, 2024 • edited Loading

eclipse-platform-bot commented Dec 19, 2024 • edited Loading

github-actions bot commented Dec 19, 2024 • edited Loading

Test Results

mickaelistria commented Dec 19, 2024

ruspl-afed commented Jan 30, 2025

HannesWell commented Jan 30, 2025

ruspl-afed commented Jan 31, 2025

ruspl-afed commented Feb 1, 2025

ruspl-afed commented Feb 4, 2025

HannesWell commented Feb 5, 2025

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Feb 7, 2025

Choose a reason for hiding this comment

HannesWell Feb 7, 2025

Choose a reason for hiding this comment

HannesWell Feb 7, 2025

Choose a reason for hiding this comment

HannesWell Feb 7, 2025

Choose a reason for hiding this comment

HannesWell Feb 7, 2025

Choose a reason for hiding this comment

HannesWell Feb 8, 2025

Choose a reason for hiding this comment

HannesWell Feb 8, 2025

Choose a reason for hiding this comment

ruspl-afed commented Dec 19, 2024 •

edited

Loading

eclipse-platform-bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading